Update pending in-person profile queries to consider enrollment expiration by aduth · Pull Request #11129 · 18F/identity-idp

aduth · 2024-08-22T12:19:04Z

🛠 Summary of changes

Updates queries for pending in-person profiles profiles to consider whether the associated enrollment is expired.

This follows #11105 (LG-14231), which updated the proofing results job to reset the profile pending timestamp for an expired enrollment. The intent with these changes is to accommodate both new and old profiles which may or may not have this value reset.

This is a (temporary?) alternative to a solution which would involve updating database records to reset the timestamp for profiles associated with expired enrollments: #11130

📜 Testing Plan

Override config/application.yml to prevent results job from automatically marking pending profiles as successful in local development:

in_person_enrollments_ready_job_cron: 0 0 29 2 1

Have both the IdP and sample application running simultaneously
In a private browsing window, go to http://localhost:9292
Change "Level of Service" to "Identity-verified"
Click "Sign in"
Create a new account
Complete identity verification using in-person proofing
Once you reach the barcode step, close your browser
Open a rails console: rails console
Manually mark the enrollment as expired, without updating associated profile (simulate behavior prior to LG-14231: Users can't access accounts after enrollment expires #11105): InPersonEnrollment.last.update(status: :expired)
Repeat Steps 2-4
Sign in with the account you created previously

Before: You'd be redirected to the account page (wrong), and the account page would raise an exception when trying to access due_date property on the pending enrollment
After: You're redirected into the identity verification flow

changelog: Bug Fixes, In-person proofing, Add check for expired enrollment in considering pending in-person proofing

Avoid conflicts on queries not expecting linked enrollment

zachmargolis · 2024-08-22T17:29:56Z

spec/factories/users.rb

-        create(:in_person_enrollment, :pending, user: user, profile: profile)
+      fully_registered
+      profiles do
+        [association(:profile, :with_pii, :in_person_verification_pending, user: instance)]


ooo love this cleanup to use the factory associations better

Yeah, to be honest it was a bit of a nightmare to figure out how to implement this correctly, but it does feel a lot more reliable / portable, and supports overriding trait attributes in a way that's not possible with the lifecycle hooks.

Same! Big fan. 🤩

zachmargolis · 2024-08-22T18:56:45Z

spec/models/profile_spec.rb

+      # See: Idv::Session#create_profile_from_applicant_with_password
+
+      before do


style, I'd remove the space between?

Suggested change

# See: Idv::Session#create_profile_from_applicant_with_password

before do

# See: Idv::Session#create_profile_from_applicant_with_password

before do

Hm, yeah. The padding line was intentional to try to "link" it more with the context block than the before block. But maybe in that case the comment should live above the context line? Not sure how I feel about this.

I guess my thinking is the context description is what we want to happen, the before block (or sometimes a let) is how we want that to happen, and in this case, the comment clarifies the how

shanechesnutt-ft · 2024-08-22T19:30:18Z

spec/models/profile_spec.rb

+      # See: Idv::Session#create_profile_from_applicant_with_password
+
+      before do
+        profile.in_person_enrollment.update(status: :establishing, profile: nil)


When would a Profile be associated with an InPersonEnrollment and the InPersonEnrollment have profile set to nil?

Does the comment above help clarify?

identity-idp/spec/models/profile_spec.rb

Lines 68 to 71 in d1a1bc7

# When marking profile as pending in-person proofing, the enrollment hasn't yet been linked to

# the profile.

#

# See: Idv::Session#create_profile_from_applicant_with_password

maybe clarify that it's still linked to the user via the in_person_enrollment.user_id?

shanechesnutt-ft · 2024-08-22T20:16:03Z

app/models/profile.rb


  def in_person_verification_pending?
-    in_person_verification_pending_at.present?
+    in_person_verification_pending_at.present? && !in_person_enrollment&.expired?


Why is InPersonEnrollment not Expired checked rather than InPersonEnrollment is pending?

That's a good call, I'll update to have them in sync.

shanechesnutt-ft · 2024-08-22T20:23:24Z

app/models/profile.rb

  def self.in_person_verification_pending
-    where.not(in_person_verification_pending_at: nil)
+    where.not(in_person_verification_pending_at: nil).
+      left_outer_joins(:in_person_enrollment).where.not(in_person_enrollment: { status: :expired })


Same question for this change.

spec/controllers/idv/sessions_controller_spec.rb

gina-yamada · 2024-08-28T15:27:37Z

@aduth Thanks for digging into this. After further investigation, Team Joy is going to move forward with updating the data (setting profile.in_person_verification_pending_at to nil in the USPS job) with the PR and then backfilling the data like what you are doing in PR, with some updates.

I think we can close this pull request now.

aduth · 2024-08-28T16:23:56Z

@aduth Thanks for digging into this. After further investigation, Team Joy is going to move forward with updating the data (setting profile.in_person_verification_pending_at to nil in the USPS job) with the PR and then backfilling the data like what you are doing in PR, with some updates.

I think we can close this pull request now.

Makes sense to me 👍

aduth · 2024-08-29T15:15:49Z

Though I'd like to salvage some of the spec factory improvements here, I'll put together a pull request for just those changes.

aduth added 8 commits August 20, 2024 11:30

Return false for IPP pending if enrollment expired

f38b276

changelog: Bug Fixes, In-person proofing, Add check for expired enrollment in considering pending in-person proofing

Exclude expired enrollments in IPP verification filter

1001121

Use left join for filtering pending IPP by non-expired enrollment

7718b83

Avoid conflicts on queries not expecting linked enrollment

Avoid redundant enrollment creation with pending IPP profile

6a8077b

Treat profile as pending if unlinked establishing enrollment

1508418

Fix spec factory relationship with nested association

6feccd1

Use factory builders to create pending IPP

26a9203

Restore with_pii trait for with_pending_in_person_enrollment profile

d1a1bc7

aduth requested review from a team and KeithNava August 22, 2024 12:19

aduth mentioned this pull request Aug 22, 2024

Backfill reset pending timestamp on profiles with expired IPP enrollment #11130

Closed

zachmargolis approved these changes Aug 22, 2024

View reviewed changes

shanechesnutt-ft reviewed Aug 22, 2024

View reviewed changes

eileen-nava reviewed Aug 22, 2024

View reviewed changes

spec/controllers/idv/sessions_controller_spec.rb Show resolved Hide resolved

eileen-nava mentioned this pull request Aug 22, 2024

Regression specs for users with cancelled and failed in_person_enrollments #11141

Merged

eileen-nava and others added 3 commits August 23, 2024 14:00

add regression spec for cancelled and failed in_person_enrollments

2894f13

Handle nil in_person_enrollment as pending

9738b03

s/build/create

b4a195a

aduth closed this Aug 28, 2024

aduth deleted the aduth-profile-expired-enrollment-ipp-pending branch August 28, 2024 16:23

eileen-nava mentioned this pull request Aug 29, 2024

Update account page #11143

Closed

h-m-m mentioned this pull request Oct 4, 2024

LG-14273: Ensure the in-person verification profile has an enrollment record #11315

Merged

		# See: Idv::Session#create_profile_from_applicant_with_password

		before do

	# When marking profile as pending in-person proofing, the enrollment hasn't yet been linked to
	# the profile.
	#
	# See: Idv::Session#create_profile_from_applicant_with_password

Conversation

aduth commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gina-yamada commented Aug 28, 2024

Uh oh!

aduth commented Aug 28, 2024

Uh oh!

aduth commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aduth commented Aug 22, 2024 •

edited

Loading

aduth Aug 22, 2024 •

edited

Loading

zachmargolis Aug 22, 2024 •

edited

Loading